[discover] Less field list loading #147825

mattkime · 2022-12-20T02:17:06Z

Summary

Looking at why Discover was loading data view field lists were loading more than needed, I found more reasons than actual field list loads occurring but I think they're all deserving of improvement.

Discover was loading the default data view to see if there was an existing data view. We can rely on the hasData api instead.
We were loading a data view and then refreshing the field list. If the data view was being loaded fresh, its meant the field list was loaded twice. Field list refresh has been integrated into the dataViews api.
We were loading a data view, checking it if was adhoc, and tossing it if it wasn't. Now we keep the reference.

Previously on page load, discover would make 3 calls to fields_for_wildcard, now it makes two. It loads all the field with one request and it makes an additional request where it applies the current filter to find relevant fields.

Closes #147744

mattkime · 2022-12-20T03:02:23Z

src/plugins/discover/public/application/main/discover_main_route.tsx

@@ -93,13 +93,6 @@ export function DiscoverMainRoute(props: Props) {
          return;
        }

-        const defaultDataView = await data.dataViews.getDefaultDataView();


This functionality duplicates what dataViews.hasData is doing. If it doesn't work, it should be fixed.

mattkime · 2022-12-20T03:04:02Z

src/plugins/discover/public/application/main/discover_main_route.tsx

@@ -111,7 +104,6 @@ export function DiscoverMainRoute(props: Props) {

        const ipList = ip.list;
        const dataViewData = resolveDataView(ip, nextSavedSearch.searchSource, toastNotifications);


refreshFields has been added to the data view loading functions - previously the data view would be loaded, triggering the first field list load, and then it would be loaded again to make sure it was fresh.

mattkime · 2022-12-20T03:04:43Z

src/plugins/discover/public/application/main/utils/resolve_data_view.ts

@@ -38,9 +38,6 @@ export function findDataViewById(
  dataViews: DataViewListItem[],
  id: string
 ): DataViewListItem | undefined {
-  if (!Array.isArray(dataViews) || !id) {
-    return;
-  }
  return dataViews.find((o) => o.id === id);


I removed this code since its enforced by type safety. Let me know if there's some reason why this shouldn't be done.

mattkime · 2022-12-20T03:07:08Z

src/plugins/discover/public/application/main/utils/resolve_data_view.ts

  // try to fetch adhoc data view first
  try {
-    const fetchedDataView = fetchId ? await dataViews.get(fetchId) : undefined;
+    fetchedDataView = fetchId ? await dataViews.get(fetchId) : undefined;


Previously this code was loading a data view, checking if it was adhoc, then tossing the result. Now we hold on to it in case its the data view we wanted to load.

mattkime · 2022-12-20T03:13:44Z

src/plugins/discover/public/application/main/utils/resolve_data_view.ts

@@ -122,12 +120,14 @@ export async function loadDataView(
  } catch (e) {}

  // fetch persisted data view
-  const actualId = getDataViewId(fetchId, dataViewList, config.get('defaultIndex'));
  return {


We don't need getDataViewId because dataViews.getDefaultDataView does the work - it provides the default data view and if it doesn't exist then it sets another default and if there aren't any data views...well, thats what the hasData check is for.

elasticmachine · 2022-12-20T03:59:28Z

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

ninoslavmiskovic · 2022-12-20T06:45:51Z

@mattkime This is a great improvement. Are we able to calculate e.g how much speed improvement this gives on a large data set ?

mattkime · 2022-12-20T13:30:32Z

@ninoslavmiskovic

Are we able to calculate e.g how much speed improvement this gives on a large data set ?

Somewhere between 50% and 33% faster, although our goal should be to load the field list without a noticeable delay.

kertal

Looking great, thx for fixing this, could you also have a look at the data view switching behavior, there are also 3 requests that could be 2

mattkime · 2022-12-21T21:28:18Z

@kertal

could you also have a look at the data view switching behavior, there are also 3 requests that could be 2

resolved!

ninoslavmiskovic · 2022-12-22T07:34:10Z

@kertal

could you also have a look at the data view switching behavior, there are also 3 requests that could be 2

resolved!

More speed 👍

kibana-ci · 2022-12-22T08:38:26Z

💚 Build Succeeded

Buildkite Build
Commit: 73636cb

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`dataViews`	230	231	+1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`discover`	418.9KB	418.6KB	-332.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`dataViews`	45.5KB	45.6KB	+110.0B
`unifiedSearch`	51.7KB	51.6KB	-43.0B
total			+67.0B

Unknown metric groups

API count

id	before	after	diff
`data`	3296	3300	+4
`dataViews`	1024	1032	+8
total			+12

ESLint disabled in files

id	before	after	diff
`osquery`	1	2	+1

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`fleet`	61	67	+6
`osquery`	109	115	+6
`securitySolution`	436	442	+6
total			+20

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`fleet`	70	76	+6
`osquery`	110	117	+7
`securitySolution`	512	518	+6
total			+21

History

💚 Build #96432 succeeded 2be0396
💔 Build #96428 failed 0cd7c17
💚 Build #95906 succeeded a4ee983
💔 Build #95905 failed 0dc8292
💔 Build #95900 failed 231ba16

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

stratoula

Unified search changes LGTM, great improvement :)

kertal

LGTM! Thx for this improvement 🙏 very appreciated!

## Summary Looking at why Discover was loading data view field lists were loading more than needed, I found more reasons than actual field list loads occurring but I think they're all deserving of improvement. - Discover was loading the default data view to see if there was an existing data view. We can rely on the `hasData` api instead. - We were loading a data view and then refreshing the field list. If the data view was being loaded fresh, its meant the field list was loaded twice. Field list refresh has been integrated into the dataViews api. - We were loading a data view, checking it if was adhoc, and tossing it if it wasn't. Now we keep the reference. Previously on page load, discover would make 3 calls to `fields_for_wildcard`, now it makes two. It loads all the field with one request and it makes an additional request where it applies the current filter to find relevant fields. Closes elastic#147744

less field list loading

231ba16

mattkime changed the title ~~less field list loading~~ [discover] Less field list loading Dec 20, 2022

mattkime commented Dec 20, 2022

View reviewed changes

fix / add jest tests

0dc8292

mattkime marked this pull request as ready for review December 20, 2022 03:55

mattkime requested a review from a team as a code owner December 20, 2022 03:55

fix test

a4ee983

kertal reviewed Dec 21, 2022

View reviewed changes

reduce field list loading when switching data views

0cd7c17

mattkime requested a review from a team as a code owner December 21, 2022 19:58

Merge branch 'main' into discover_less_field_list_loading

2be0396

Merge branch 'main' into discover_less_field_list_loading

73636cb

mattkime requested a review from kertal December 22, 2022 07:00

stratoula approved these changes Dec 22, 2022

View reviewed changes

kertal approved these changes Dec 22, 2022

View reviewed changes

mattkime merged commit 4715e71 into elastic:main Dec 22, 2022

kibanamachine added the backport:skip This commit does not require backporting label Dec 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[discover] Less field list loading #147825

[discover] Less field list loading #147825

mattkime commented Dec 20, 2022 •

edited

Loading

mattkime Dec 20, 2022

mattkime Dec 20, 2022

mattkime Dec 20, 2022

mattkime Dec 20, 2022

mattkime Dec 20, 2022 •

edited

Loading

elasticmachine commented Dec 20, 2022

ninoslavmiskovic commented Dec 20, 2022

mattkime commented Dec 20, 2022

kertal left a comment

mattkime commented Dec 21, 2022

ninoslavmiskovic commented Dec 22, 2022

kibana-ci commented Dec 22, 2022

API count

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

stratoula left a comment

kertal left a comment

		@@ -111,7 +104,6 @@ export function DiscoverMainRoute(props: Props) {

		const ipList = ip.list;
		const dataViewData = resolveDataView(ip, nextSavedSearch.searchSource, toastNotifications);

[discover] Less field list loading #147825

[discover] Less field list loading #147825

Conversation

mattkime commented Dec 20, 2022 • edited Loading

Summary

mattkime Dec 20, 2022

Choose a reason for hiding this comment

mattkime Dec 20, 2022

Choose a reason for hiding this comment

mattkime Dec 20, 2022

Choose a reason for hiding this comment

mattkime Dec 20, 2022

Choose a reason for hiding this comment

mattkime Dec 20, 2022 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Dec 20, 2022

ninoslavmiskovic commented Dec 20, 2022

mattkime commented Dec 20, 2022

kertal left a comment

Choose a reason for hiding this comment

mattkime commented Dec 21, 2022

ninoslavmiskovic commented Dec 22, 2022

kibana-ci commented Dec 22, 2022

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Async chunks

Page load bundle

API count

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

History

stratoula left a comment

Choose a reason for hiding this comment

kertal left a comment

Choose a reason for hiding this comment

mattkime commented Dec 20, 2022 •

edited

Loading

mattkime Dec 20, 2022 •

edited

Loading